fix(cryptography): support php builds without argon2#2165
Conversation
|
@brendt would we be fine with a breaking change that would use bcrypt by default? We can't automate config changes unfortunately, and this fallback implementation is not really ideal. As far as I know, Argon2 support is in most default PHP installations, so we can also just specify in the docs that it's required for Tempest to run by default |
|
@innocenzi whats the reason to use Argon2 over bcrypt? Side note to keep in mind: in theory, this would have been less of an issue if we published configurations. By not publishing configurations these things become breaking changes. I'd suggest we just slot this for next major and document for the time being. |
Mainly because it would be a breaking change otherwise. Also, my understanding is that argon2 underperforms compared to bcrypt only if it's not properly configured, it's not worse than bcrypt per-se.
I'm confused here—do you mean that Tempest shouldn't have its internal config files? Would be really bad UX if we force-published them in the default install imo.
Fine by me, but I really don't think we have to replace argon2 by bcrypt |
The claim, I believe, is that Argon2 is weaker for faster runtimes, which I think we probably are.
I'm conflicted. I get the DX side of the argument, but think about it... one change to a config file setting could throw your whole app off if you aren't overriding. |
|
I read that claim, but my understanding is still that it's a configuration issue, and that it's not easy to configure / there is no good default to recommend. But granted, I'm not very knowledgeable in this area. If this wasn't for the breaking change, I'd be fine reverting to bcrypt anyway.
I don't think it's that big of a deal, I'd take that tradeoff for the benefits it brings. Now that I think about it, the breaking change could be mitigated by having our upgrade command publish a config with argon2, so the new default config could use bcrypt without it being breaking? |
HashingAlgorithmtook its case values fromPASSWORD_ARGON2ID/PASSWORD_BCRYPT, butPASSWORD_ARGON2IDonly exists on PHP builds compiled with Argon2. Without it the enum can't load, which also kills the bcrypt path (BcryptConfigreferencesHashingAlgorithm::BCRYPT) and the defaulthashing.config.php(new ArgonConfig()) hits a fatal on boot before any bcrypt override applies. The enum now uses the literal strings behind those constants ('argon2id','2y') - the same valuespassword_hash()accepts andpassword_get_info()returns.The default config falls back to bcrypt only when Argon2 isn't available, so Argon2id stays the default on every build that supports it and the framework boots on the ones that don't.
Fixes #2147